-
Notifications
You must be signed in to change notification settings - Fork 632
TF2.16 support: hermetic python shim, TF 2.16.2 pin, legacy Keras path, setup/packaging fixes #913
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
TF2.16 support: hermetic python shim, TF 2.16.2 pin, legacy Keras path, setup/packaging fixes #913
Conversation
mhucka
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the work, and I'm sorry about the number of changes requested here …
| fi | ||
|
|
||
| mkdir -p ${DEST} | ||
| mkdir -p "${DEST}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trivial nit: something happened to the intendentation on this line.
| mkdir -p "${DEST}" | |
| mkdir -p "${DEST}" |
| "Programming Language :: Python :: 3.7", | ||
| "Programming Language :: Python :: 3.8", | ||
| "Programming Language :: Python :: 3.9", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should omit these, since these versions are no longer actively supported.
| "Programming Language :: Python :: 3.7", | |
| "Programming Language :: Python :: 3.8", | |
| "Programming Language :: Python :: 3.9", |
| "Programming Language :: Python :: 3.8", | ||
| "Programming Language :: Python :: 3.9", | ||
| "Programming Language :: Python :: 3.10", | ||
| "Programming Language :: Python :: 3.11", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's also add 3.12 and 3.13 (assuming the updated TFQ from this PR works in those versions):
| "Programming Language :: Python :: 3.11", | |
| "Programming Language :: Python :: 3.11", | |
| "Programming Language :: Python :: 3.12", | |
| "Programming Language :: Python :: 3.13", |
| author="Google Inc.", | ||
| author_email="no-reply@google.com", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this file is getting changes for other reasons, it may as well also include an update to these values. The general approach we've taken in other major QAI projects is to use the following pattern, so I think it's worth including in this PR so that another PR isn't needed to make this change later.
| author="Google Inc.", | |
| author_email="no-reply@google.com", | |
| author="The TensorFlow Quantum Authors", | |
| author_email="tensorflow-quantum-team@google.com", |
| # pylint: enable=undefined-variable | ||
|
|
||
| __version__ = '0.7.2' | ||
| __version__ = '0.7.5' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, sorry I didn't notice this before: the next version is actually 0.7.4 (since the current version on PyPI is 0.7.3):
| __version__ = '0.7.5' | |
| __version__ = '0.7.4' |
|
|
||
| function is_windows() { | ||
| # On windows, the shell script is actually running in msys | ||
| is_windows() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| is_windows() { | |
| function is_windows() { |
|
|
||
| function is_ppc64le() { | ||
| [[ "$(uname -m)" == "ppc64le" ]] | ||
| write_legacy_python_repo() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| write_legacy_python_repo() { | |
| function write_legacy_python_repo() { |
| PY_ABS="$("${PY}" - <<'PY' | ||
| import os,sys | ||
| print(os.path.abspath(sys.executable)) | ||
| PY | ||
| )" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although this works, IMHO this construct is fragile because of the complicated syntax. (At first, I thought the )" by itself at the end was a typo.) Would the following not work?
| PY_ABS="$("${PY}" - <<'PY' | |
| import os,sys | |
| print(os.path.abspath(sys.executable)) | |
| PY | |
| )" | |
| PY_ABS="$("${PY}" -c 'import os,sys; print(os.path.abspath(sys.executable))')" |
| "${PYTHON_BIN_PATH}" - <<'PY' || { echo "ERROR: tensorflow not importable by chosen Python."; exit 1; } | ||
| import tensorflow as tf | ||
| import tensorflow.sysconfig as sc | ||
| print("TF:", tf.__version__) | ||
| print("include:", sc.get_include()) | ||
| print("lib:", sc.get_lib()) | ||
| PY | ||
|
|
||
| # --- discover TF include/lib robustly -------------------------------------- | ||
| HDR="$("$PYTHON_BIN_PATH" - <<'PY' | ||
| import tensorflow.sysconfig as sc | ||
| print(sc.get_include()) | ||
| PY | ||
| )" | ||
|
|
||
| LIBDIR="$("$PYTHON_BIN_PATH" - <<'PY' | ||
| import os, tensorflow.sysconfig as sc | ||
| p = sc.get_lib() | ||
| print(p if os.path.isdir(p) else os.path.dirname(p)) | ||
| PY | ||
| )" | ||
|
|
||
| LIBNAME="$("$PYTHON_BIN_PATH" - <<'PY' | ||
| import os, glob, tensorflow.sysconfig as sc | ||
| p = sc.get_lib() | ||
| d = p if os.path.isdir(p) else os.path.dirname(p) | ||
| cands = glob.glob(os.path.join(d,'libtensorflow_framework.so*')) \ | ||
| or glob.glob(os.path.join(d,'libtensorflow.so*')) \ | ||
| or glob.glob(os.path.join(d,'_pywrap_tensorflow_internal.*')) | ||
| print(os.path.basename(cands[0]) if cands else 'libtensorflow_framework.so.2') | ||
| PY | ||
| )" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The execution of this is slow because it imports tensorflow 4 times. It's true that the imports after the first time will be faster thanks to caching by the operating system, but still, it feels like there must be a more efficient way than running Python 4 times. Also, there is a little bit of duplication of code.
What about the following?
| "${PYTHON_BIN_PATH}" - <<'PY' || { echo "ERROR: tensorflow not importable by chosen Python."; exit 1; } | |
| import tensorflow as tf | |
| import tensorflow.sysconfig as sc | |
| print("TF:", tf.__version__) | |
| print("include:", sc.get_include()) | |
| print("lib:", sc.get_lib()) | |
| PY | |
| # --- discover TF include/lib robustly -------------------------------------- | |
| HDR="$("$PYTHON_BIN_PATH" - <<'PY' | |
| import tensorflow.sysconfig as sc | |
| print(sc.get_include()) | |
| PY | |
| )" | |
| LIBDIR="$("$PYTHON_BIN_PATH" - <<'PY' | |
| import os, tensorflow.sysconfig as sc | |
| p = sc.get_lib() | |
| print(p if os.path.isdir(p) else os.path.dirname(p)) | |
| PY | |
| )" | |
| LIBNAME="$("$PYTHON_BIN_PATH" - <<'PY' | |
| import os, glob, tensorflow.sysconfig as sc | |
| p = sc.get_lib() | |
| d = p if os.path.isdir(p) else os.path.dirname(p) | |
| cands = glob.glob(os.path.join(d,'libtensorflow_framework.so*')) \ | |
| or glob.glob(os.path.join(d,'libtensorflow.so*')) \ | |
| or glob.glob(os.path.join(d,'_pywrap_tensorflow_internal.*')) | |
| print(os.path.basename(cands[0]) if cands else 'libtensorflow_framework.so.2') | |
| PY | |
| )" | |
| tf_output=$("${PYTHON_BIN_PATH}" - <<'PY' | |
| import sys | |
| import os | |
| import glob | |
| try: | |
| import tensorflow as tf | |
| import tensorflow.sysconfig as sc | |
| except ImportError: | |
| sys.exit(1) | |
| print(sc.get_include()) | |
| lib_path = sc.get_lib() | |
| lib_dir = lib_path if os.path.isdir(lib_path) else os.path.dirname(lib_path) | |
| print(lib_dir) | |
| cands = (glob.glob(os.path.join(lib_dir, 'libtensorflow_framework.so*')) or | |
| glob.glob(os.path.join(lib_dir, 'libtensorflow.so*')) or | |
| glob.glob(os.path.join(lib_dir, '_pywrap_tensorflow_internal.*'))) | |
| print(os.path.basename(cands[0]) if cands else 'libtensorflow_framework.so.2') | |
| PY | |
| ) | |
| if [[ $? -ne 0 ]]; then | |
| echo "ERROR: tensorflow not importable by Python (${PYTHON_BIN_PATH})" >&2 | |
| exit 1 | |
| fi | |
| { | |
| read -r HDR | |
| read -r LIBDIR | |
| read -r LIBNAME | |
| } <<< "${tf_output}" |
|
|
||
| load("@org_tensorflow//tensorflow:workspace0.bzl", "tf_workspace0") | ||
|
|
||
| tf_workspace0() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the removal of this call to tf_workspace0() intentional?
Summary
Key changes
--python=flag; generate .tf_configure.bazelrc + .bazelrc + the python shim${PYTHON_BIN_PATH:-python3}and ensure setuptools presentTesting
bazel build ... release:build_pip_packagesucceedsNotes